Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Access single file or folder from IStorageFolder by name #17771

Merged
merged 2 commits into from
Jan 23, 2025

Conversation

nickodei
Copy link
Contributor

@nickodei nickodei commented Dec 14, 2024

What does the pull request do?

This PR addresses #17276. It allows to access a single file or folder by name given you have already a folder (IStorageFolder).

What is the current behavior?

You can currently archive this by iteration over the whole folder with GetItemsAsync() and pick the file with the given name. This has performance considerations and therefore, when applicable, the implementations tries to access the file directly.

What is the updated/expected behavior with this PR?

When you get a folder (IStorageFolder) e.g through a file-picker, you can access the child file/folder by name directly, returning IStorageFile or IStorageFolder depending on the called method.

How was the solution implemented (if it's not obvious)?

The interface IStorageFolder was extended as follows:

interface IStorageFolder
{
     Task<IStorageFolder> GetFolderAsync(string name);
     Task<IStorageFile> GetFileAsync(string name);
}

Therefore, an implementation for the following platforms was provided:

  • Android (DocumentsContract and the query method)
  • Browser (FileSystemDirectoryHandle)
  • Desktop (.NET Path.Combine)
  • IOS (.NET Path.Combine => NSUrl)

Testing

Tested it on following platforms:

  • Desktop
  • Android
  • Browser
  • IOS (dont have an IOS device)

Extended the ControlCatalog for testing:

image

Checklist

Breaking changes

No breaking changes, this only extends the public interface

Fixed issues

Fixes #17276

@nickodei nickodei marked this pull request as draft December 14, 2024 15:47
@nickodei
Copy link
Contributor Author

I was not able to have an Android-Implementation that does not iterate trough the whole folder to find the first Item with the given name. The other thing is, that I don't have an IOS device to test the IOS-implementation.

@nickodei
Copy link
Contributor Author

nickodei commented Dec 18, 2024

Questions:

  • Should I return IStorageFolder or IStorageFolder? ? => When nullable, which Excetions should be catched and which should be thrown (e.g getDirectoryHandle throws 4 different Exceptions. Should I catch all of them and return null?)
  • Can I write tests for it or is it enough that I manually tested it?
  • How to I resolve the build error?

@nickodei nickodei marked this pull request as ready for review December 18, 2024 15:25
@MrJul MrJul added enhancement needs-api-review The PR adds new public APIs that should be reviewed. labels Jan 14, 2025
@MrJul
Copy link
Member

MrJul commented Jan 14, 2025

  • Should I return IStorageFolder or IStorageFolder? ? => When nullable, which Excetions should be catched and which should be thrown (e.g getDirectoryHandle throws 4 different Exceptions. Should I catch all of them and return null?)

We'll discuss that in an API review meeting soon and let you know, but I think the return type should be nullable. After all, even the Create{File/Folder}Async methods can return null.

  • Can I write tests for it or is it enough that I manually tested it?

Platform specific code is unfortunately hard to test, and we don't currently have integration tests for storage, so manual testing should be ok here.

  • How to I resolve the build error?

Adding new members to an interface is a breaking change, which is what the CI build reports here. However, IStorageFolder is marked with NotClientImplementable so we exceptionally allow breaking changes here as only platforms should implement it.

You should run nuke ValidateApiDiff to generate an API suppression file (but you might want to wait until we've properly reviewed the new API).

@MrJul
Copy link
Member

MrJul commented Jan 14, 2025

Diff for API review:

 namespace Avalonia.Platform.Storage {
 {
     [NotClientImplementable]
     public interface IStorageFolder : IStorageItem
     {
+        System.Threading.Tasks.Task<IStorageFile?> GetFileAsync(System.String name);
+        System.Threading.Tasks.Task<IStorageFolder?> GetFolderAsync(System.String name);
     }
 }

@nickodei
Copy link
Contributor Author

Thank you for your response. I'm still down to finish this MR so I will wait until the API review.

@MrJul
Copy link
Member

MrJul commented Jan 21, 2025

Notes from the API review meeting:

The API shape is good.

However, all platforms should behave the same, as much as possible.
For example, on the browser, there's an exception explicitly thrown at the start of the method (wrong copy/paste?), which should return null instead.

@MrJul MrJul added api-approved The new public APIs have been approved. and removed needs-api-review The PR adds new public APIs that should be reviewed. labels Jan 21, 2025
@nickodei nickodei force-pushed the get-single-storage-item-from-folder branch from 8121655 to 0535cb6 Compare January 21, 2025 16:55
@nickodei nickodei force-pushed the get-single-storage-item-from-folder branch from 0535cb6 to 3f449a4 Compare January 21, 2025 17:34
@nickodei
Copy link
Contributor Author

Thanks. I incorporated the browser change and updated the api-diff. It seems a bit weird because throwing vs. returning null is not everywhere strictly enforced when using the nullable operator. Should I just catch all Exceptions and return null? Event in the browser example, while I return null (if it is not a directory), there is still the potential to throw an Exception (wrong access, etc. ). But for the most part is shouldn't.

At @MrJul: Are you able to review my changes?

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.3.999-cibuild0054385-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@MrJul
Copy link
Member

MrJul commented Jan 22, 2025

Thanks. I incorporated the browser change and updated the api-diff. It seems a bit weird because throwing vs. returning null is not everywhere strictly enforced when using the nullable operator. Should I just catch all Exceptions and return null? Event in the browser example, while I return null (if it is not a directory), there is still the potential to throw an Exception (wrong access, etc. ). But for the most part is shouldn't.

We should model this on the BclStorageFolder/Item. Those are using Exists, which return false if any IOException or UnauthorizedAccessException occurs: we'll never throw in normal cases for this implementation (we still throw for OOM or other really unexpected errors).

Other implementations should match this behavior. Return null if the file can't be listed.
Bcl, Android and iOS all seem correct.
Browser needs swallowing the unauthorized access exception in the current catch block.

@nickodei
Copy link
Contributor Author

nickodei commented Jan 22, 2025

Other implementations should match this behavior. Return null if the file can't be listed.
Bcl, Android and iOS all seem correct.
Browser needs swallowing the unauthorized access exception in the current catch block.

Got it and also re-tested the browser implementation.

There where also some other JS-errors throws that I needed to suppress (NotFound and TypeMissmatch) to match the behavior of the other implementation.

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.3.999-cibuild0054441-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

Copy link
Member

@MrJul MrJul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you!

@MrJul MrJul added this pull request to the merge queue Jan 23, 2025
Merged via the queue into AvaloniaUI:master with commit fce5682 Jan 23, 2025
10 checks passed
@MrJul MrJul added feature and removed enhancement labels Jan 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-approved The new public APIs have been approved. feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Access single file using BclStorageFolder
3 participants